fix spectral extraction previews on unit change #3157
Conversation
github-actions
bot
added
cubeviz
specviz
plugin
| flux_unit = Unicode().tag(sync=True) | ||
| sb_unit = Unicode().tag(sync=True) | ||
| flux_units = Unicode().tag(sync=True) | ||
| sb_units = Unicode().tag(sync=True) |
There was a problem hiding this comment.
just to be consistent with the traitlets above, I was confusing myself when writing code later in the plugin and having to switch between singular and plural
| @@ -542,7 +545,9 @@ def _preview_x_from_extracted(self, extracted): | |||
| return extracted.spectral_axis.value | |||
|
|
|||
| def _preview_y_from_extracted(self, extracted): | |||
| return extracted.flux.value | |||
| # TODO: use extracted's PIXAR_SR instead (but for some reason isn't populated here...) | |||
There was a problem hiding this comment.
does anyone know why this is (I'm guessing its just populated later when actually loading into the app)? Ultimately, I think the extracted spectrum just gets a copy of PIXAR_SR from the input cube, so this shouldn't matter, it just would read more cleanly if it did.
kecnry
force-pushed
the
spec-ext-previews
branch
2 times, most recently
from
56ed577
to
67df70a
Compare
kecnry
requested review from
rosteen,
javerbukh,
pllim,
bmorris3,
haticekaratay,
cshanahan1 and
gibsongreen
as code owners
|
It seems like changing to flux makes the preview disappear (as expected, I think), but then changing back to surface brightness doesn't trigger the preview to reappear. It only comes back if you then change the flux unit while in surface brightness mode. |
|
is it possible that its timing out? Do you get the warning that the previews have been disabled? |
|
The previews disabled warning does not appear, I do see the warning about the display units (surface brightness vs flux). And now that I think about it, I guess the preview should show up when flux is selected in unit conversion. It doesn't seem to be timing out. I was able to get the preview to show this time. You can see that at first it flickered and then disappeared when I switched to flux, and when I changed units (the second time?) it stuck around: Screen.Recording.2024-08-21.at.4.52.46.PM.mov |
kecnry
force-pushed
the
spec-ext-previews
branch
from
7b7d1a7
to
8149cf4
Compare
kecnry
force-pushed
the
spec-ext-previews
branch
from
8149cf4
to
5c781f1
Compare
|
@cshanahan1 - I think this is fixed now after a rebase, but please confirm if you have a second to test. |
|
(still working on some cases where the preview is not showing after the rebase) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3157 +/- ##
==========================================
+ Coverage 88.46% 88.47% +0.01%
==========================================
Files 125 125
Lines 18677 18700 +23
==========================================
+ Hits 16522 16545 +23
Misses 2155 2155 ☔ View full report in Codecov by Sentry. |
kecnry
force-pushed
the
spec-ext-previews
branch
from
5c781f1
to
9e56b9f
Compare
There was a problem hiding this comment.
seems to work as intended - I can't convert to erg/s/cm2/a but i think that will be fixed by mine and @pllim PRs
Description
This pull request updates the spectral extraction live-preview for any change that affects the y-units of the extracted spectrum or spectral viewer (and also ensures the internal units in the plugin are populated for the warning banner introduced in #3143 which was recently broken).
Screen.Recording.2024-08-19.at.2.28.41.PM.mov
Change log entry
CHANGES.rst? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rstbefore merge. If no, maintainershould add a
no-changelog-entry-neededlabel.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
triviallabel.